Add field for changing target port if app gateway fails to start #50982
Conversation
ravicious
added
the
no-changelog
ravicious
force-pushed
the
r7s/change-target-port
branch
from
1ea0115 to
8cbbd1b
Compare
This will enable each callsite to establish it's own rules as to which fields are required. This will be useful once we add required target subresource name for TCP apps.
ravicious
force-pushed
the
r7s/change-target-port
branch
from
8cbbd1b to
f266bf6
Compare
| LocalPort = 'localPort', | ||
| TargetSubresourceName = 'targetSubresourceName', | ||
| } | ||
| type FormFieldNames = `${FormFields}`; |
There was a problem hiding this comment.
Yeah, too much magic. 😩 But I wanted to use string enums here and this seems to be the most succinct way of getting the actual values out of the enum.
|
|
||
| await user.click(screen.getByText('Reconnect')); | ||
|
|
||
| expect(await screen.findByLabelText('Terminal input')).toBeInTheDocument(); |
There was a problem hiding this comment.
What is this 'Terminal input'?
There was a problem hiding this comment.
Oh, I forgot to add a comment about it. When I was writing tests, I saw in the error output that xterm adds aria-label equal to "Terminal Input" somewhere. I thought it'd be a good way of checking if the terminal is rendered on screen. I think in the past we might have used some other checks for that?
There was a problem hiding this comment.
I think in the past we might have used some other checks for that?
Ah yes, I think we used some other checks for that. aria-label is definitely better.
| return; | ||
| } | ||
|
|
||
| reconnect(parseResult.data); |
There was a problem hiding this comment.
No pushback on the zod approach? I think it sucks!!! But I wasn't able to come up with anything better.
It'd likely be easier to use and think about if it was better integrated with our existing stuff (e.g. form components), but I didn't want to invest too much time into it upfront.
There was a problem hiding this comment.
No pushback on the zod approach? I think it sucks!!!
Yeah, it's quite complex, but I didn't have a better idea too.
But now that I think about it, I'm wondering if we need that flexibility at all. The form data is passed to the reconnect function which has a defined type. This means that the callsites need to use identical field names. If so, why don't we render the controls inside the OfflineGateway?
Or, OTOH the argument of reconnect should infer the type from the schema.
There was a problem hiding this comment.
But now that I think about it, I'm wondering if we need that flexibility at all. The form data is passed to the
reconnectfunction which has a defined type. This means that the callsites need to use identical field names. If so, why don't we render the controls inside theOfflineGateway?
Because different gateways store different data under targetSubresourceName. DB gateways have a db name there while app gateways have a target port. For now it of course doesn't matter, as OfflineGateway would show the field for targetSubresourceName only for app gateways. So technically we could get away with using just another prop for the target subresource name field.
There was a problem hiding this comment.
Since you already invested time in this, I would keep the current implementation, maybe it will make our lives easier in the future. I would only update the reconnect type.
There was a problem hiding this comment.
You mean the reconnect prop for OfflineGateway? I was going to say that I'm not sure if we should change it, as technically every OfflineGateway callsite is expected to pass the same reconnect function. But I just now realized that DocumentGatewayKube doesn't use useGateway.
There was a problem hiding this comment.
You mean the reconnect prop for OfflineGateway?
Yes. Now it's type is (args: { localPort?: string }): void but we actually pass targetSubresourceName there as well. Technically, we can just add it there as an optional property, but maybe we could easily infer the whole type from formSchema?
There was a problem hiding this comment.
Instead of inferring it from schema, I made it so that both formSchema and reconnect are expected to operate on the same type which can include any combination of available form names. Let me know what you think.
ravicious
force-pushed
the
r7s/change-target-port
branch
from
2ca17ea to
1ee9abf
Compare
|
@ravicious See the table below for backport results.
|
) * Make OfflineGateway use uncontrolled forms * Pass form fields to OfflineGateway from outside This will enable each callsite to establish it's own rules as to which fields are required. This will be useful once we add required target subresource name for TCP apps. * Add tests for submitting form through OfflineGateway * Make sure reconnect and formSchema operate on the same type * Add comment for Terminal input
This builds on #50912. One of the issues that I haven't taken care of in that PR is that if a target port you created a gateway for is no longer defined in the app spec, then when restoring tabs Connect wouldn't be able to create such gateway, as tshd would return an error about the port not being valid.
This PR adds a Target Port field next to the existing Local Port field. When a gateway fails to start, the user is able to change the target port, just like they're able to change the local port today.
To achieve this, I refactored
OfflineGateway. It used to support a prop which dictated whether the Local Port is going to be shown or not, along with another prop to set the default port and another for the label for the input field. Now it expects its callers to pass uncontrolled form fields throughrenderFormControlsandformSchema. When the form is submitted,OfflineGatewayruns the form data through the schema and then passes the result tocreateGateway. This way each caller is able to specify by themselves what fields its going to use and how it's going to validate the form:DocumentGatewayKubedoesn't use any fields.DocumentGateway(db gateways) shows just the local port field which is optional.DocumentGatewayAppshows the local port field. If the app is a multi-port TCP app, it also shows the target port field.targetSubresourceNameis always present, since that's how we can tell gateway docs for multi-port apps from single-port apps.zod usage
Uncontrolled forms are naturally not "typeable" since it's just a bunch of strings, so I used zod to transform form data into an object with the expected fields. As long as the form schema is properly defined, the component is going to throw an error if the form is missing one of the fields.
I have to say that I just now realized that using zod with strict null checks off is suboptimal, as in that scenario it's impossible to make zod return a type with non-optional fields. It's not that big of an issue here, but it might be if we use this approach elsewhere. On top of that, zod's errors require some extra work if we want to show them in the UI. For now I just skipped this work since all errors here are the result of a programmer error, not the user submitting invalid data. But in the future, our form components could be adjusted to show zod errors.
Considered alternatives
Another prop for target port field
Just like
OfflineGatewayhadgatewayPort: { isSupported: boolean, defaultPort: string }, it could have an equivalent of this prop for the target subresource name that stores the target port for app gateways. I ruled out this solution as different gateways treat target subresource name in different ways and this design wouldn't be very flexible.Controlled forms
Instead of using zod and uncontrolled forms,
renderFormControlscould receiveuseStatereturn values for each field and then build form controls using that. However,OfflineGatewaywould still need to receive props with defaults for each field, as it'd be controlling the fields.Honestly, maybe it wouldn't be that bad. The extra props for default values seem kinda off, but otherwise we wouldn't need to deal with zod. While it'd be impossible to introduce bugs stemming from incorrect input names, this design introduces a potential issue where
renderFormControlsdoes not render an expected field and there's no error about it. But in the end it just means that it'd required tests, just like the zod implementation does to ensure that the form schemas are in sync with the forms.Button for removing the gateway
This is probably the solution I should've went with in the first place. The target port being no longer valid is probably going to be such a niche use case that it feels wrong to dedicate so much time and code to taking care of it. Just remove the gateway and let user create a new one, this time with a correct port.
But well, I've already invested the time and code into this. 🥲 I suppose if we ever find ourselves in need of extending gateway support, e.g. choosing kube namespaces, maybe parts of this implementation will make things easier.